-
Notifications
You must be signed in to change notification settings - Fork 985
Download cleanup #4531
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Download cleanup #4531
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM modulo some questions :)
Please note that since this PR is related to more subtle changes in the user behavior that is not fully covered by the test suite, more attention should be paid to ensure the necessary preservation of original semantics. |
Do you think there's anything left to do here, or is it okay to merge this? |
@djc I think it should be okay to merge this patch now that the concerns have been addressed. If there are further problems we can fix them separately later. |
Followup from:
Sorry @FranciscoTGouveia, here are some more changes which IMO make things a bunch cleaner. I haven't done any testing yet (would be great if you want to do some of that) but I think the changes here are relatively straightforward?